Repository navigation
fix(core): wrap observer-object handlers like functions, and let a wrapped promise settle after teardown - #3829
Conversation
Firebase accepts a subscription handler either as a function or as an object with next, error and complete members (data-connect spells them onNext, onErr, onComplete). The zone wrapper only recognized the function, so an observer object reached the SDK untouched: no pending task held server rendering, inject() threw inside the handler, and in a zone.js app the handler ran outside the Angular zone, where the wrapper itself had moved the call. The object is now replaced by a copy whose function members are wrapped the same way, with this bound to the caller's object. onConfigUpdate takes only the object form and its handler fires only on a remote template change, so it joins the blockUntilFirst: false list along with onTokenChanged, whose first callback a first visit may never request. Both would otherwise hold the application unstable forever once their observers are wrapped.
When a wrapped call returns a Promise, the wrapper resolved its own promise through runInInjectionContext and NgZone.run. Neither reaches the caller: a .then callback runs as a later microtask after the injection context is torn down, and zone.js runs it in the zone where .then was attached, not where resolve was called. The only effect the two layers had was on teardown. runInInjectionContext throws once its injector is destroyed, so a Firebase promise settling after ApplicationRef.destroy() or a TestBed reset threw before resolve ran and the caller's promise never settled. The pending task still opens on the call and closes on settlement.
tyler-reitz
left a comment
There was a problem hiding this comment.
Approving. A fresh build regenerates both files byte-identical to the PR, Chrome headless passes 140/140, and rebuilding with main's src/zones.ts fails exactly the 14 zones.spec.ts cases the body names.
Non-blocking nits:
- The PR title is
fix:but both commits usefix(core):, which is what the squash should carry. Worth retitling before merge. src/zones.tsadds/* */comments among the existing//ones. The content is worth keeping; only the style differs.isObserverLikereads members withReflect.get, so it also matches anext,errororcompleteinherited through the prototype, and any wrapped call's object argument that has one gets copied. I don't know of a Firebase argument type like that, but I haven't audited every wrapped signature, so this is a question rather than a finding.
The previous commit treated any object argument with a next, error or complete method as an observer. A model class with a complete() method, saved through setDoc with a converter, was replaced by a plain copy that lost its prototype, and its pending task was never released. tools/build.ts now reads Firebase's type declarations with the TypeScript compiler API and passes a fourth ɵzoneWrap argument, true, only to the 11 exports with an observer-shaped parameter. Every other function receives its object arguments untouched. A handler Firebase calls after the app is destroyed now runs directly, because entering a destroyed injector throws NG0205. Handlers passed as functions already hit this on main.
# Conflicts: # src/app-check/firebase.ts # src/remote-config/firebase.ts # src/zones.spec.ts # tools/build.ts
|
Thanks for the review, @tyler-reitz. Your third point turned up a bug this PR introduced, so the branch has changed since your approval. Can you take another look? TitleRetitled to Comment styleThe
|
tyler-reitz
left a comment
There was a problem hiding this comment.
Re-approving on 731b063 after the third commit and the merge.
Checked locally: a build reproduces the generated firebase.ts files with no drift, karma passes 169/169, and removing the acceptsObserver guard and the injector.destroyed check fails the 7 specs the description says it should.
Non-blocking: the six observer member names live in both src/zones.ts and tools/build.ts, held together only by the "must match" comment. Fine to leave; worth a shared constant if either list ever changes.
Fixes #3828 will close the issue on merge, so the 21.1.x backport needs tracking somewhere else.
…rotask angular#3829's destroyed-injector early return now sits inside the try, so the finally removes the pending task on that path too. The argument loop is main's, without the TODO this branch deleted. angular#3829's spec comment about a 0 ms timer now gives the reason the per-spec counter still holds. # Conflicts: # src/zones.spec.ts # src/zones.ts
angular#3829 wraps a handler passed as an observer object, such as { next, error }, the way it wraps a function, so the injection context reaches both forms. Refs angular#3828
angular#3829 set onConfigUpdate to blockUntilFirst: false in tools/build.ts, because its observer runs only when a fetched template differs from the active one. The list in docs/zones.md left it out. Found in the angular#3835 review. Refs angular#3829
* docs: say which wrapped calls do not hold the app unstable docs/zones.md says callback APIs hold the app unstable until an initial value, so a server render waits for data. #3827 makes a set of calls exceptions because their first callback may never come. Waiting on a callback that never comes would keep the app from ever becoming stable. List those calls in docs/zones.md, and point at onValue, object, list, listVal and fromRef with the value event as the calls that still make a server render wait. In docs/database.md, say the same under stateChanges, auditTrail and fromRef. Refs #3826 * docs: describe firebase 13 support in the v22 upgrade guide #3831 lets AngularFire 22 accept firebase 12 or 13, and the ng update migration now writes the range of the firebase major already installed. The upgrade guide still said the migration writes ^12.18.0 and quoted peer warnings that name only that range. State the supported range and what the migration writes, update the Yarn 1 and pnpm 9/10 warnings to their current wording, and add a section on moving to firebase 13: VertexAIBackend is removed and its packages declare Node 24.12 or later. Refs #3830 * docs: describe microtask delivery in the v22 upgrade guide #3833 moves every Observable read from a setInterval timer to the next microtask checkpoint. As a breaking change it needs a section in the upgrade guide: which tests now see a value they used to wait for, the setTimeout ordering flip, and the zoneless server render case for hand-written TransferState entries. Refs #3832 * docs: describe the callback pending-task change in the v22 upgrade guide #3837 removes a callback read's pending task right after the callback instead of one setTimeout(fn, 0) delay later. As a breaking change it needs a section in the upgrade guide: what moves sooner, and which tests notice. Refs #3836 * docs: say a handler passed as an observer object is wrapped too #3829 wraps a handler passed as an observer object, such as { next, error }, the way it wraps a function, so the injection context reaches both forms. Refs #3828 * docs(compat): add a setup section in NgModule and standalone form The compat API's setup steps left docs/install-and-setup.md when #3236 rewrote it for the modular API, and nothing replaced them. docs/compat.md had no setup at all, the Realtime Database pages pointed at an "Installation guide" that no longer covered compat, and no page showed the standalone form. A standalone app with only the modular providers then fails its first compat injection with NG0201 for angularfire2.app.options (#3683). The new section on docs/compat.md gives both forms, says when the modular providers may stay, states the error the missing module produces, and lists the compat module of each product. The twelve compat pages that had an NgModule-only snippet or no setup at all now point at it. Fixes #3683 * docs: say which package managers refuse an incompatible firebase The v22 upgrade guide said an incompatible firebase is refused at install time. Only npm refuses it, and only when package.json requests the incompatible version. Yarn and pnpm install it with a warning. * docs: say which Firestore functions are not zone wrapped #3842 re-exports Firestore's query and reference builders, field values, comparators and setLogLevel without the zone wrapper. In docs/zones.md, add a paragraph listing them, change the switchMap example to a read, since building the query no longer warns, and say the Verbose log level covers only wrapped calls. Refs #3842 * docs(auth): build the server app with initializeBrowserOrServerApp The server-rendering guide's step 4 becomes the one call, the five details become three, the App Check token gets a relay section, the response gets a Cache-Control header, and the manual callback stays in a "Building the app yourself" section that step 4 points at, for apps that prefer it or whose AngularFire version has no such function. The release modes say what a throwing ngOnDestroy does to a timed release. * docs(auth): describe the sample as an example of the one-call server app The sample app now has one provideFirebaseApp with initializeBrowserOrServerApp in its shared app.config.ts (#3845), so the sentence about its split client and server files no longer holds. Refs #3843 * docs: say how to move from 22.0.0-rc.0 in the v22 upgrade guide The guide covered upgrades from 21, a 21 release candidate and a 22 canary build, but not from the first 22 release candidate. ng add saves a prerelease as an exact version, so an app on 22.0.0-rc.0 stays there until ng update @angular/fire@next moves it. That command also runs migration-v22 again, which reports "No changes made" for a firebase entry already inside the supported range. Refs #3834 * docs(auth): say which hooks stop a releaseAfterRender release The caveat on releaseAfterRender said a throwing ngOnDestroy "stops that teardown". It now says the release is a DestroyRef.onDestroy callback, that Angular runs it after every service's and component's ngOnDestroy, and that an ngOnDestroy method or an ApplicationRef.onDestroy listener that throws keeps it from running. The default release, which does not run during the injector's destroy, is unaffected either way. A new paragraph says to call initializeBrowserOrServerApp in app.config.ts rather than a route's providers: a route's child injector is not destroyed with the request, so a release registered there never runs. Refs #3844 * docs(auth): say the default release still happens from a route's providers Tyler asked on #3844 whether the default release, now tied to a root service, releases a server app built in a lazy route's providers. It does: the request's root injector is destroyed, the route's child injector is not, so only a releaseAfterRender release registered there is lost. Checked with a child environment injector that is never destroyed, in both modes. Refs #3844 * docs: add onConfigUpdate to the calls that do not hold the app unstable #3829 set onConfigUpdate to blockUntilFirst: false in tools/build.ts, because its observer runs only when a fetched template differs from the active one. The list in docs/zones.md left it out. Found in the #3835 review. Refs #3829 * docs(auth): say a second provideFirebaseApp can render signed out The bullet said a second registration makes a signed-in render fail on a freshly started server. With more than one registration, FirebaseApp resolves to the default app. Run on fresh servers, a second plain initializeApp(config) registration creates that app on the first request, so signed-in pages render signed out with no error. Only when no registration creates the default app, as with two initializeBrowserOrServerApp registrations, does the render fail, and only until a signed-out request creates it. Found in the #3835 review. Refs #3844
Fixes #3828
ɵzoneWrapnow wraps a handler passed as an observer object the way it wraps a handler passed as a function, for the 11 functions whose declared parameters include an observer. Firebase receives a copy of the object whosenext,error,complete,onNext,onErrandonCompletemembers are wrapped and bound to the caller's object.tools/build.tsfinds those 11 at build time by reading Firebase's type declarations with the TypeScript compiler API, and passes them a fourthɵzoneWrapargument,true. Every other wrapped function passes its object arguments through untouched.onConfigUpdate(remote-config) getsblockUntilFirst: false, because wrapping its observer attruewould hold the application unstable until a config update a normal load may never produce.runInInjectionContextandNgZone.runlayers that never reached the caller and threw once the injector was destroyed.NG0205, in both forms.Checklist
ɵzoneWrapignores a handler passed as an observer object, and holds a wrapped promise forever once its injector is destroyed #3828docs/zones.md, saying the re-entry holds for a handler passed as an object too, goes in the rc.1 docs PR, docs: document the 22.0.0-rc.1 changes #3835.src/zones.spec.tshas fifteen cases, each run underprovideZoneChangeDetection()and underprovideZonelessChangeDetection().src/firestore/firestore.spec.tshas one more, against the realsetDocand the emulator.yarn install,yarn testrun successfully?: yes, with npm:npm ci,npm run build,npm test(which runsbuild:jasmine,test:node-esmandtest:chrome-headless), plusnpm run test:nodeandnpm run lint.Changes
Before this change the argument loop in
ɵzoneWraptestedtypeof _arguments[i] === 'function'and nothing else, so an observer object reached the SDK untouched. The issue lists the eleven affected functions and what each form lost.The first commit:
isObserverLike, true for a non-null object with a function under any of the six member names, andzoneWrapObserver, which returns a copy of the object with those members replaced by the same wrapper a function argument gets..bind(observer)keepsthisfor a class instance or an RxJSSubject.onConfigUpdate: { blockUntilFirst: false, logLevel: LogLevel.WARN }to the remote-config block oftools/build.ts.onTokenChanged, which fix: stop twelve wrapped calls from holding the app unstable #3827 also added onmain, so after the merge only theonConfigUpdateentry is new.The second commit:
ret.then((it) => runInInjectionContext(injector, () => run(() => resolve(it))), ...)withret.then(resolve, reject).assertNotDestroyedcheck is what hung a promise settling after teardown (the issue has the details).The third commit:
complete()method, saved throughsetDocwith a converter, reached the converter as a plain copy without its prototype, and the call's pending task was never released.exportsTakingAnObservertotools/build.ts. It type-checks an in-memory file that imports each Firebase entry point, and keeps the exports with a parameter whose type is an object with a function-valued observer member.ɵzoneWrap(fn, blockUntilFirst, logLevel, true).ɵzoneWraplooks for an observer object only when that fourth argument istrue.zoneWrapFncall the handler directly wheninjector.destroyedis true. Before, a handler Firebase called afterApplicationRef.destroy()or aTestBedreset threwNG0205, which already happened onmainfor handlers passed as functions.The merge of
main:isSupportedentry and this PR'sonConfigUpdateentry.src/zones.spec.ts, which both sides created, keeps both sets of specs.What callers can notice
onSnapshot(ref, { next })and the other object-form calls now hold server rendering until the first value, run their members inside the Angular zone, and can callinject()inside them, exactly like the function form.onConfigUpdate, which never held the application unstable (its only form was never wrapped), does not start to now that its observer is.setDocincluded, receives its object arguments exactly as before this PR.inject()inside it are the same as before.NG0205.Verification
injector.destroyedcheck fromzoneWrapFnfails the new teardown spec in both modes withNG0205: Injector has already been destroyed.ɵzoneWrapfails five specs: the two data-object cases in both modes, and the FirestoresetDocconverter spec.npm run test:nodeandnpm run test:node-esm: 401 specs, 0 failures each.npm run lintpasses.@angular/fire/firestoreand@angular/fire/remote-configentry points: the object form matches the function form on pending task, zone andinject()in both apps, andonConfigUpdateregisters no task. This was rerun with the build-time flag in place, before theNG0205change and the merge.getDocregisters and releases its task and resolves with the document, a rejecting wrapped call releases its task and the rejection reaches the caller, the caller's continuation sees the same zone and the sameinject()failure as before, and a wrapped call made inside a childEnvironmentInjectorthat is destroyed before the promise settles resolves.Backport
Non-breaking.
src/zones.tson21.1.xis identical to this branch's starting point. A cherry-pick PR against21.1.xfollows at a later date, after a run on an Angular 21 app.